-
Notifications
You must be signed in to change notification settings - Fork 420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow the CMake project to use system-wide installed dependencies #35
Conversation
Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired. Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mosra -- changes in PR look good to me. Left one minor nit comment about what seems to be a mis-indendation.
Checked default compilation on MacOS 10.14.1 through build.sh
but didn't try using system-wide dependencies. I do think we should expose the option to build with system dependencies as that will help dev experience and build times.
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Fixes a warning related to GLVND on my machine.
This however means we have to bundle a few Find modules here, instead of picking them up from the submodules directly.
Uncovered when switching to system dependencies, with the bundled ones it wasn't complaining as the target already existed.
Rebased on current master to fix conflicts with now-merged #29. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Builds for me on MacOS and ubuntu -- without system dependencies
Looks useful for project quality of life. Very clean PR. Thank you for introducing |
…cebookresearch#35) * Add .editorconfig. * CMake: let Magnum find OpenGL. Fixes a warning related to GLVND on my machine. * CMake: make it possible to use system packages for most dependencies. This however means we have to bundle a few Find modules here, instead of picking them up from the submodules directly. * esp/bindings: minor update to make things work with stable pybind11. * esp/nav: forward-declare a struct as a struct. * esp/gfx: properly find the Application library. Uncovered when switching to system dependencies, with the bundled ones it wasn't complaining as the target already existed.
Motivation and Context
Simplify workflow for developers who have some of the dependencies installed system-wide -- making full rebuild times shorter and allowing changes to dependent projects to be made without
git submodule update
(done either by accident or as part ofsetup.py
) destroying them again.Together with that a bunch of other minor things. Let me know if it's okay like this or I should submit these separately.
Note: I did not update any docs, as I'm not sure where to put these (or if the new CMake options are any useful for the general public).
How Has This Been Tested
Creating a custom CMake build folder with desired options specified and then directing
setup.py
to use that. While testing I realized a few further modifications could be made tosetup.py
to further simplify this workflow. These are however fully orthogonal to these changes, so I will submit them as a separate PR.I suggest reviewing commit-by-commit as each commit message describes each change in more detail. Also, please point out all crimes against coding style etc. I made :)
Types of changes
Checklist